New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monorepo: add package.json linter #36534
Conversation
7f65930
to
9b08888
Compare
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
@@ -51,7 +51,7 @@ The only exception are `devDependencies` which _must be declared in the wp-calyp | |||
"module": "dist/esm/index.js", | |||
"sideEffects": false, | |||
"keywords": [ "wordpress" ], | |||
"author": "Your Name <you@example.com> (https://yoursite.wordpress.com/)", | |||
"author": "Automattic Inc.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is better so as not to promote too tight "code ownership". Thoughts?
I can ping individuals who's names I've switched to the company name in this PR once it's in a mergeable state.
There is separate "contributors" array if folks really would like to have their names here. https://docs.npmjs.com/files/package.json#people-fields-author-contributors
I also tried to see if using org handle @automattic
as an author gives any extra features in npm
CLI or http://npmjs.org/ but it doesn't seem to.
9b08888
to
0dd0980
Compare
0dd0980
to
afb5c0c
Compare
@@ -0,0 +1,49 @@ | |||
module.exports = { | |||
// See https://www.npmjs.com/package/@wordpress/npm-package-json-lint-config | |||
extends: '@wordpress/npm-package-json-lint-config', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, just list all those rules here and have one package less to Renovate/update. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂ How much value do we get value from extending? I'm inclined to just list our own rules here, it's pretty trivial to pull in or remove a rule when it makes sense, while keeping the package up to date adds overhead and it can be surprising when rules shift around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much value do we get value from extending?
Whenever anything changes/gets deprecated in the upstream package, we'd inherit those changes. We have that problem with our Stylelint config - a bunch of it was outdated last time I checked.
@gziolo does it make sense to use PS. might be new |
Sure, this is how we use it internally in Gutenberg as well:
Cool, see WordPress/gutenberg#18054 :) |
65f1a15
to
bd091fa
Compare
Rebased |
@@ -0,0 +1,49 @@ | |||
module.exports = { | |||
// See https://www.npmjs.com/package/@wordpress/npm-package-json-lint-config | |||
extends: '@wordpress/npm-package-json-lint-config', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂ How much value do we get value from extending? I'm inclined to just list our own rules here, it's pretty trivial to pull in or remove a rule when it makes sense, while keeping the package up to date adds overhead and it can be surprising when rules shift around.
ac3cba6
to
044fc52
Compare
Co-Authored-By: Jon Surrell <jon.surrell@automattic.com>
05e39d9
to
42834ea
Compare
rules: { | ||
'prefer-no-devDependencies': 'error', | ||
'prefer-property-order': 'off', | ||
'require-bugs': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still my favorite rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to land. Anything else you want to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint monorepo
package.json
files.Uses neat
npm-package-json-lint
for the job.Linter config based on
@wordpress/npm-package-json-lint-config
and customized a bit to our needs, with a few overrides.Changes proposed in this Pull Request
package.json
linter, not only to keep monorepo packages consistent but especially to keepdevDependencies
out frompackages/**/package.json
since they can cause actual issues.Testing instructions
npm run lint:package-json
Linter runs as part of
npm run lint
as well.